Allow Ranges::contains to accept (e.g.) &str for Ranges<String>
#35
Conversation
charliermarsh
force-pushed
the
charlie/bv
branch
from
712ff6a to
c19c102
Compare
CodSpeed Performance ReportCongrats! CodSpeed is installed 🎉
You will start to see performance impacts in the reports once the benchmarks are run from your default branch.
|
version-ranges/src/lib.rs
Outdated
| self.segments | ||
| .binary_search_by(|segment| { | ||
| // We have to reverse because we need the segment wrt to the version, while | ||
| // within bounds tells us the version wrt to the segment. | ||
| within_bounds(version, segment).reverse() | ||
| within_bounds(version.borrow(), segment).reverse() |
There was a problem hiding this comment.
Interesting, I expected you to need segment.borrow() here. Why does this work... hmmm.
There was a problem hiding this comment.
e.g., Below, you are doing start.borrow() and not version.borrow(), which is what I'd expect.
There was a problem hiding this comment.
This confused me too... but the inverse doesn't work.
There was a problem hiding this comment.
Did you expect that I'd need both segment.borrw() and version.borrow()?
There was a problem hiding this comment.
No, just segment.borrow(). I think I'd have to play around with this to figure it out.
There was a problem hiding this comment.
Oh ok. That's a hint that I can pull on then.
There was a problem hiding this comment.
It turns out I don't need either...? Which I think makes sense given the impl of within_bounds?
There was a problem hiding this comment.
Oooooo derp, yes. I was missing within_bounds, and that's where you do the borrow() call and it is done on the thing I expect.
(This pattern from HashMap inverts the typical use of Borrow, where you call borrow() on the parameter to match what is stored. But here, you want to call borrow() on the thing stored to match what is provided.)
| @@ -1326,7 +1334,7 @@ pub mod tests { | |||
|
|
|||
| #[test] | |||
| fn from_range_bounds(range in any::<(Bound<u32>, Bound<u32>)>(), version in version_strat()) { | |||
| let rv: Ranges<_> = Ranges::from_range_bounds(range); | |||
| let rv: Ranges<_> = Ranges::<u32>::from_range_bounds(range); | |||
There was a problem hiding this comment.
I guess this means this is technically a breaking change. (Although we technically permit type inference regressions as semver compatible in Rust-library-land... Unless they are too disruptive.) I only mention this because I think version-ranges is published as a library?
There was a problem hiding this comment.
Agreed that this is technically a breaking changes, but i expect that real users use their Ranges as parameters to a function that has the generic parameter fix (the DependencyProvider has), so as an inference change I consider it acceptable.
charliermarsh
force-pushed
the
charlie/bv
branch
from
c19c102 to
74d0029
Compare
## Summary A small TODO that I found interesting. See: astral-sh/pubgrub#35.
…#35) ## Summary This PR borrows a trick from [HashMap](https://doc.rust-lang.org/std/collections/struct.HashMap.html#method.contains_key) to enable users to pass (e.g.) `&str` to `Ranges::contains`, given `Ranges<String>`.
Summary
This PR borrows a trick from HashMap to enable users to pass (e.g.)
&strtoRanges::contains, givenRanges<String>.